Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature #37611

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

shgutte
Copy link
Contributor

@shgutte shgutte commented Feb 17, 2025

Description of Problem/Feature:
Previously, we used a normal OTA file to update the 917 SoC for both the Application image and TA processor image. We could combine these updates into a single OTA image only. With the introduction of multi-OTA, we added a new tag in the header of the OTA file to detect the OTA processor that will process the image. This allows us to update multiple processor images using OTA in one go. In future, if we add another co-processor, we will be able to update all processor images at once using single OTA.

This PR introduces functionality to support arbitrary image downloads in a single OTA file. It can use pre-defined TLVs or custom TLVs. The ota_image_tool.py script has been updated to create a custom OTA file with TLVs embedded.

Testing

Before the Change:

  • The OTA update process was limited to updating the multiple processors separately.
  • The OTA file did not have a tag to detect the OTA processor, limiting the ability to process multiple images simultaneously.

After the Change:

  • The OTA file now includes a new tag in the header to detect the OTA processor, enabling the processing of multiple images in a using single OTA file.
  • The ota_image_tool.py script successfully creates a custom OTA file with embedded TLVs.
  • Tested on the 917 SoC, confirming that the new multi-OTA functionality works as expected.
  • Verified that both the Application image and TA processor image can be updated either individually or combined on the 917 SoC using the TLV tag.
  • Future-proofed the OTA process to support additional co-processors, ensuring scalability. (Not tested it yet because don't have another processor on 917 SoC)

Copy link

semanticdiff-com bot commented Feb 17, 2025

Review changes with  SemanticDiff

Changed Files
File Status
  examples/platform/silabs/SiWx917/BUILD.gn Unsupported file format
  scripts/tools/silabs/ota/ota_multi_image_tool.py  0% smaller
  src/platform/silabs/SiWx917/BUILD.gn Unsupported file format
  src/platform/silabs/efr32/BUILD.gn Unsupported file format
  src/platform/silabs/multi-ota/OTAHooks.cpp Unsupported file format
  src/platform/silabs/multi-ota/OTAMultiImageProcessorImpl.cpp Unsupported file format
  src/platform/silabs/multi-ota/OTATlvProcessor.cpp Unsupported file format
  src/platform/silabs/multi-ota/OTATlvProcessor.h Unsupported file format
  src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp Unsupported file format
  src/platform/silabs/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h Unsupported file format
  third_party/silabs/efr32_sdk.gni Unsupported file format

@shgutte shgutte force-pushed the feature/wifi/multi-ota-processor branch from 80b36ac to 293f69c Compare February 24, 2025 07:27
@shgutte shgutte changed the title [WIP][Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature [Silabs] [WiFi] Added changes for the 917 Wi-Fi multi-ota feature Feb 24, 2025
@shgutte shgutte marked this pull request as ready for review February 24, 2025 07:31
@shgutte shgutte requested a review from a team as a code owner February 24, 2025 07:31
Copy link

PR #37611: Size comparison from 43a8a9b to 66e2d0c

Full report (1 build for stm32)
platform target config section 43a8a9b 66e2d0cf change % change
stm32 light STM32WB5MM-DK FLASH 459800 459800 0 0.0
RAM 141472 141472 0 0.0

@@ -62,6 +62,7 @@ class TAG:
APPLICATION = 1
BOOTLOADER = 2
FACTORY_DATA = 3
WIFI_917_TA_M4 = 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIFI_917_TA_M4 why are we using device specific tag name here?

@@ -163,6 +164,32 @@ def generate_app(args: object):
return [OTA_APP_TLV_TEMP, args.app_input_file]


def generate_wifi_image(args: object):
"""
Generate app payload with descriptor. If a certain option is not specified, use the default values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment needs to be updated

"""
logging.info("App descriptor information:")

descriptor = generate_descriptor(args.app_version, args.app_version_str, args.app_build_date)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this required? looks to be application specific


return CHIP_NO_ERROR;
}

CHIP_ERROR chip::OTAMultiImageProcessorImpl::OtaHookInit()
{
static chip::OTAFirmwareProcessor sApplicationProcessor;
static chip::OTAFactoryDataProcessor sFactoryDataProcessor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sFactoryDataProcessor why was this required before and not now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is present in the code just went down in the macros which are there for WIFI and EFR


auto & imageProcessor = chip::OTAMultiImageProcessorImpl::GetDefaultInstance();

#ifndef SLI_SI91X_MCU_INTERFACE
static chip::OTAFirmwareProcessor sApplicationProcessor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is sApplicationProcessor not required for SLI_SI91X_MCU_INTERFACE, the Si917 should also have an application processor right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because 917 SoC directly support the combined image so we don't need to implement the separate processor for it

kWiFiProcessor = 4,
kCustomProcessor1 = 8,
kCustomProcessor2 = 9,
kCustomProcessor3 = 10,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a limit or bound for the enum would be recommended for which we can test if the OTAProcessorTag is not unbound.

@@ -0,0 +1,176 @@
/*
*
* Copyright (c) 2023 Project CHIP Authors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Copyright (c) 2023 Project CHIP Authors
* Copyright (c) 2025 Project CHIP Authors

@@ -0,0 +1,54 @@
/*
*
* Copyright (c) 2023 Project CHIP Authors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Copyright (c) 2023 Project CHIP Authors
* Copyright (c) 2025 Project CHIP Authors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file looks to be a generic WiFi Firmware Processor, why keep it under SiWx917?

Copy link
Contributor

@mkardous-silabs mkardous-silabs Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doesn't seem like we need to have a seperate wi-fi firmware processor. We should try to make it common for all platforms.

@@ -547,12 +547,14 @@ template("efr32_sdk") {
]
}

if (chip_enable_multi_ota_encryption && chip_enable_multi_ota_requestor) {
defines += [ "SL_MATTER_ENABLE_OTA_ENCRYPTION=1" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason to rename SL_MATTER_ENABLE_OTA_ENCRYPTION?

Copy link
Contributor

@mkardous-silabs mkardous-silabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR a step in the right direction but it adds a lot fo what seems to be duplication with the existing feature.

Is is possible to reduce the amount of defines and duplication between the existing code and the newly added code?

Comment on lines +78 to +87
if (chip_enable_multi_ota_requestor) {
sources += [
"${silabs_platform_dir}/multi-ota/OTAHooks.cpp",
"${silabs_platform_dir}/multi-ota/OTAMultiImageProcessorImpl.cpp",
"${silabs_platform_dir}/multi-ota/OTAMultiImageProcessorImpl.h",
"${silabs_platform_dir}/multi-ota/OTATlvProcessor.cpp",
"${silabs_platform_dir}/multi-ota/OTATlvProcessor.h",
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp",
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h",
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move these GN changes to a BUILD.gn within the multi-ota directory and have a depency in the efr32 and SiwX917.


OTADataAccumulator mAccumulator;
bool mDescriptorProcessed = false;
#if OTA_ENCRYPTION_ENABLE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define has changed - This won't work anymore.

Copy link
Contributor

@mkardous-silabs mkardous-silabs Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doesn't seem like we need to have a seperate wi-fi firmware processor. We should try to make it common for all platforms.

@@ -547,12 +547,14 @@ template("efr32_sdk") {
]
}

if (chip_enable_multi_ota_encryption && chip_enable_multi_ota_requestor) {
defines += [ "SL_MATTER_ENABLE_OTA_ENCRYPTION=1" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is reverting the define to its previous version when this was part for the provision structure refactor.
Can you remove this change?

Comment on lines +46 to +55
CHIP_ERROR OTAWiFiFirmwareProcessor::Init()
{
VerifyOrReturnError(mCallbackProcessDescriptor != nullptr, CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED);
mAccumulator.Init(sizeof(Descriptor));
#if OTA_ENCRYPTION_ENABLE
mUnalignmentNum = 0;
#endif //OTA_ENCRYPTION_ENABLE

return CHIP_NO_ERROR;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as the Non-Wifi processor. We should move this to header to make it common and reduce duplication.

Comment on lines +160 to +165
#ifdef SLI_SI91X_MCU_INTERFACE // This is not needed for the 917 SoC; it is required for EFR host applications
// send system reset request to reset the MCU and upgrade the m4 image
ChipLogProgress(SoftwareUpdate, "SoC Soft Reset initiated!");
// Reboots the device
sl_si91x_soc_nvic_reset();
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. The SLI_SI91X_MCU_INTERFACE define is only present for the 917 SoC.

#define RPS_HEADER 1
#define RPS_DATA 2

uint8_t flag = RPS_HEADER;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be global?

@@ -163,6 +164,32 @@ def generate_app(args: object):
return [OTA_APP_TLV_TEMP, args.app_input_file]


def generate_wifi_image(args: object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is a copy paste of the generate app function outside of the tag change.

Why do we need a specific tag to the M4 / TA image?
If we do need a specific tag, we should modify the function to take the tag as input instead.

Comment on lines +345 to +346
create_parser.add_argument('-wifi', "--wifi-input-file",
help='Path to OTA image for SiWx917 (TA/M4/Combined file), 917 NCP (TA)')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't need a specific tag, we don't need this change either.

Comment on lines +108 to +113
if (chip_enable_wifi) {
sources += [
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.cpp",
"${silabs_platform_dir}/multi-ota/SiWx917/OTAWiFiFirmwareProcessor.h",
]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these for all NCP combos?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants